Normalize datetime types for unified query API by dai-chen · Pull Request #5408 · opensearch-project/sql

dai-chen · 2026-05-06T02:34:16Z

Description

Add postAnalysisRules to LanguageSpec.LanguageExtension and register DatetimeExtension in UnifiedPplSpec with two post-analysis rules:

DatetimeUdtNormalizeRule: Rewrites PPL datetime UDT return types (EXPR_DATE/TIME/TIMESTAMP) on RexCall nodes to standard Calcite types, enabling downstream consumers to process the plan without UDT-related failures.
DatetimeOutputCastRule: Adds a final LogicalProject that casts standard datetime output columns to VARCHAR, aligning with PPL's wire-format contract (ISO string representation).

Examples

Case 1: UDT Normalize Rule — source=events | eval d = DATE(name) | fields d

 Before:  LogicalProject(d=[DATE($1):EXPR_DATE])
 After:   LogicalProject(d=[DATE($1):DATE])

Case 2: Output Cast Rule — source=events | fields hire_date, start_time, created_at

 Before:  LogicalProject(hire_date=[$2], start_time=[$3], created_at=[$4])
            LogicalTableScan(table=[[events]])

 After:   LogicalProject(hire_date=[CAST($0):VARCHAR], start_time=[CAST($1):VARCHAR], created_at=[CAST($2):VARCHAR])
            LogicalProject(hire_date=[$2], start_time=[$3], created_at=[$4])
              LogicalTableScan(table=[[events]])

Notes

The UDT normalize rule rewrites datetime function return types to standard Calcite types, but a mismatch remains with the underlying PPL UDF implementations (which still operate on String values). This does not affect the Analytics Engine path (reimplements UDFs in DataFusion) but will fail on the UnifiedQueryCompiler path. Follow-up PR will address function implementations.
Currently, the output cast rule uses CAST(datetime AS VARCHAR) whose string format is engine-dependent: PPL Calcite produce ANSI SQL format (2024-01-15 12:00:00) like most other SQL databases (SparkSQL, PostgreSQL, MySQL, Oracle, SQL Server), while DataFusion produces ISO 8601 format (2024-01-15T12:00:00).

Related Issues

Part of #5250

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
New PPL command checklist all confirmed.
API changes companion pull request created.
Commits are signed per the DCO using --signoff or -s.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2026-05-06T02:36:11Z

PR Reviewer Guide 🔍

(Review updated until commit `af2b288`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
📝 TODO sections api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [55]: Generate optimal physical plan to fully unify query execution and leverage Calcite's optimizer.
🔀 Multiple PR themes Sub-PR theme: Add postAnalysisRules extension point and OpenSearch datetime precision Relevant files: core/src/main/java/org/opensearch/sql/executor/OpenSearchTypeSystem.java api/src/main/java/org/opensearch/sql/api/spec/LanguageSpec.java Sub-PR theme: Implement DatetimeExtension with UDT normalize and output cast rules Relevant files: api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeExtension.java api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeUdtNormalizeRule.java api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeOutputCastRule.java api/src/main/java/org/opensearch/sql/api/spec/UnifiedPplSpec.java api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java api/src/test/java/org/opensearch/sql/api/spec/datetime/DatetimeExtensionTest.java
⚡ Recommended focus areas for review Stateful Singleton `DatetimeOutputCastRule.INSTANCE` is a singleton but `RelHomogeneousShuttle` may carry mutable traversal state. If the shuttle accumulates state across multiple `visit` calls (e.g., parent/child tracking), concurrent or sequential reuse of the singleton could produce incorrect results. Verify that `RelHomogeneousShuttle` is truly stateless before using a shared singleton. static final DatetimeOutputCastRule INSTANCE = new DatetimeOutputCastRule(); Stateful Singleton Same concern as `DatetimeOutputCastRule`: `DatetimeUdtNormalizeRule.INSTANCE` is a shared singleton of `RelHomogeneousShuttle`. If the base class holds any mutable traversal state, reusing the same instance across multiple `plan()` calls could cause subtle bugs. Confirm thread-safety and statelessness. static final DatetimeUdtNormalizeRule INSTANCE = new DatetimeUdtNormalizeRule(); Missing Recursion `visit(RelNode other)` does not call `super.visit(other)` before processing, so child nodes are never visited. The cast wrapping is only applied to the root node passed in. If the plan has nested projections with datetime fields that are not at the root, they will not be cast. Compare with `DatetimeUdtNormalizeRule` which correctly calls `super.visit(other)` first. public RelNode visit(RelNode other) { List<RelDataTypeField> fields = other.getRowType().getFieldList(); if (fields.stream().noneMatch(f -> isDatetimeType(f.getType().getSqlTypeName()))) { return other; } RexBuilder rexBuilder = other.getCluster().getRexBuilder(); List<RexNode> projects = new ArrayList<>(fields.size()); List<String> names = new ArrayList<>(fields.size()); // Cast datetime fields to VARCHAR for output; pass through others unchanged for (RelDataTypeField field : fields) { RexNode newField = rexBuilder.makeInputRef(other, field.getIndex()); RelDataType fieldType = field.getType(); if (isDatetimeType(fieldType.getSqlTypeName())) { projects.add(castToVarchar(rexBuilder, newField, fieldType)); } else { projects.add(newField); } names.add(field.getName()); } return LogicalProject.create(other, List.of(), projects, names); } UDT Nullable Mismatch `UdtMapping.fromUdtType` checks `instanceof AbstractExprRelDataType<?>` but does not handle the case where the UDT type is wrapped in a nullable type decorator. Depending on how Calcite wraps nullable types, the instanceof check may fail for nullable UDT columns, silently skipping normalization. static Optional<UdtMapping> fromUdtType(RelDataType type) { if (!(type instanceof AbstractExprRelDataType<?> e)) { return Optional.empty(); } ExprUDT udt = e.getUdt(); return Arrays.stream(values()).filter(u -> u.udtType == udt).findFirst(); } Rule Order Dependency Post-analysis rules are applied sequentially in registration order. `DatetimeUdtNormalizeRule` must run before `DatetimeOutputCastRule` because the cast rule checks for standard `SqlTypeName` datetime types (not UDTs). If the order is ever changed or a new extension registers rules in between, the output cast rule may miss UDT-typed columns. This ordering dependency is implicit and not enforced. for (var shuttle : context.getLangSpec().postAnalysisRules()) { plan = plan.accept(shuttle); } return plan;

github-actions · 2026-05-06T02:36:40Z

PR Code Suggestions ✨

Latest suggestions up to af2b288

Explore these optional code suggestions:

Category Suggestion Impact

Possible issue

Recursively visit child nodes before applying cast rule

The visit method does not recursively visit child nodes before applying the cast
rule. This means datetime fields in non-root nodes (e.g., sub-queries or nested
projections) will not be processed. The method should call super.visit(other) or
visitChildren(other) first to ensure the entire tree is traversed.

api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeOutputCastRule.java [31-53]

 @Override
 public RelNode visit(RelNode other) {
-  List<RelDataTypeField> fields = other.getRowType().getFieldList();
+  RelNode visited = visitChildren(other);
+  List<RelDataTypeField> fields = visited.getRowType().getFieldList();
   if (fields.stream().noneMatch(f -> isDatetimeType(f.getType().getSqlTypeName()))) {
-    return other;
+    return visited;
   }
-  ...
-  return LogicalProject.create(other, List.of(), projects, names);
+
+  RexBuilder rexBuilder = visited.getCluster().getRexBuilder();
+  List<RexNode> projects = new ArrayList<>(fields.size());
+  List<String> names = new ArrayList<>(fields.size());
+
+  for (RelDataTypeField field : fields) {
+    RexNode newField = rexBuilder.makeInputRef(visited, field.getIndex());
+    RelDataType fieldType = field.getType();
+    if (isDatetimeType(fieldType.getSqlTypeName())) {
+      projects.add(castToVarchar(rexBuilder, newField, fieldType));
+    } else {
+      projects.add(newField);
+    }
+    names.add(field.getName());
+  }
+  return LogicalProject.create(visited, List.of(), projects, names);
 }

Suggestion importance[1-10]: 6

__

Why: The DatetimeOutputCastRule is designed as an output cast rule (applied to the root/top-level node), so not recursing into children may be intentional. However, if the rule is meant to wrap all datetime fields at every level, the missing visitChildren call could cause incomplete processing. The suggestion is valid but the intent of the rule (root-only vs. full-tree) is ambiguous from context.

Low

General

Assert all matching calls match expected type

The assertCallType helper only finds the first matching RexCall by operator name,
but multiple calls with the same operator name may exist in the plan (e.g., two
DATE(...) calls). If the first found call does not match the expected
type/precision, the assertion will fail misleadingly. Consider collecting all
matching calls and asserting that at least one (or all) match the expected type.

api/src/test/java/org/opensearch/sql/api/spec/datetime/DatetimeExtensionTest.java [196-224]

 private static void assertCallType(
     RelNode plan, String operatorName, SqlTypeName expectedType, int expectedPrecision) {
-  AtomicReference<RexCall> ref = new AtomicReference<>();
+  List<RexCall> found = new ArrayList<>();
   plan.accept(
       new RelHomogeneousShuttle() {
         @Override
         public RelNode visit(RelNode other) {
           RelNode visited = super.visit(other);
           visited.accept(
               new RexShuttle() {
                 @Override
                 public RexNode visitCall(RexCall call) {
-                  if (ref.get() == null
-                      && call.getOperator().getName().equalsIgnoreCase(operatorName)) {
-                    ref.set(call);
+                  if (call.getOperator().getName().equalsIgnoreCase(operatorName)) {
+                    found.add(call);
                   }
                   return super.visitCall(call);
                 }
               });
           return visited;
         }
       });
+  assertFalse("No RexCall found for: " + operatorName, found.isEmpty());
+  for (RexCall call : found) {
+    assertEquals(operatorName + " type", expectedType, call.getType().getSqlTypeName());
+    if (expectedPrecision >= 0) {
+      assertEquals(operatorName + " precision", expectedPrecision, call.getType().getPrecision());
+    }
+  }
+}

Suggestion importance[1-10]: 4

__

Why: The suggestion improves test robustness by checking all matching RexCall nodes rather than just the first one, which could catch cases where some calls have incorrect types. However, this is a test-quality improvement with limited impact on correctness of the production code.

Low

Previous suggestions

Suggestions up to commit cc90d7f

Category	Suggestion	Impact
Possible issue	Prevent recursive application of output cast rule The rule visits every `RelNode` in the tree but only wraps the root — it should only apply to the top-level node, not recursively to all nodes. As written, `visit(RelNode` `other)` is called for every node in the plan tree (because `RelHomogeneousShuttle` recurses via `super.visit`), so inner nodes that happen to have datetime fields will also get a cast projection injected, corrupting the plan. The rule should not call `super.visit(other)` to recurse into children, or it should be applied only to the root node externally. api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeOutputCastRule.java [31-53] @Override public RelNode visit(RelNode other) { + // Do NOT recurse into children; this rule is intended only for the root output node. List<RelDataTypeField> fields = other.getRowType().getFieldList(); if (fields.stream().noneMatch(f -> isDatetimeType(f.getType().getSqlTypeName()))) { return other; } - ... + + RexBuilder rexBuilder = other.getCluster().getRexBuilder(); + List<RexNode> projects = new ArrayList<>(fields.size()); + List<String> names = new ArrayList<>(fields.size()); + + for (RelDataTypeField field : fields) { + RexNode newField = rexBuilder.makeInputRef(other, field.getIndex()); + RelDataType fieldType = field.getType(); + if (isDatetimeType(fieldType.getSqlTypeName())) { + projects.add(castToVarchar(rexBuilder, newField, fieldType)); + } else { + projects.add(newField); + } + names.add(field.getName()); + } return LogicalProject.create(other, List.of(), projects, names); } Suggestion importance[1-10]: 7 __ Why: The `DatetimeOutputCastRule` extends `RelHomogeneousShuttle` which recurses into children by default via `super.visit()`. Since the rule doesn't call `super.visit(other)`, it actually doesn't recurse — but the suggestion raises a valid concern about intent. However, looking at the code, `visit(RelNode other)` does NOT call `super.visit(other)`, so it won't recurse into children. The suggestion misreads the code but the underlying concern about applying casts to inner nodes is worth noting if the implementation were to change.	Medium
Possible issue	Avoid shared mutable shuttle singleton for thread safety `RelHomogeneousShuttle` (and `RelShuttle` implementations in general) are not thread-safe because they maintain mutable traversal state. Sharing a single `INSTANCE` across concurrent query planning calls can lead to race conditions. Each call to `postAnalysisRules()` should return a new instance instead of a shared singleton. api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeUdtNormalizeRule.java [29] -static final DatetimeUdtNormalizeRule INSTANCE = new DatetimeUdtNormalizeRule(); +// Remove INSTANCE singleton; instantiate per use in DatetimeExtension.postAnalysisRules() +// In DatetimeExtension: +// return List.of(new DatetimeUdtNormalizeRule(), new DatetimeOutputCastRule()); Suggestion importance[1-10]: 6 __ Why: The `INSTANCE` singleton pattern for `RelHomogeneousShuttle` subclasses can be problematic in concurrent environments since shuttles may maintain traversal state. The suggestion to instantiate per use is valid for thread safety, though the actual risk depends on whether the shuttle maintains mutable state during traversal.	Low
General	Guard against null plan after shuttle application `plan.accept(shuttle)` returns a `RelNode` but `RelShuttle.accept` may return the same node or a new one. If a shuttle returns `null` (e.g., due to an unhandled node type), the next iteration will throw a `NullPointerException`. Add a null-check or assert after each shuttle application to fail fast with a meaningful error. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [66-70] RelNode plan = strategy.plan(query); for (var shuttle : context.getLangSpec().postAnalysisRules()) { - plan = plan.accept(shuttle); + RelNode result = plan.accept(shuttle); + if (result == null) { + throw new IllegalStateException( + "Post-analysis rule " + shuttle.getClass().getSimpleName() + " returned null plan"); + } + plan = result; } return plan; Suggestion importance[1-10]: 3 __ Why: While adding a null-check is a defensive practice, `RelShuttle` implementations in Calcite are not expected to return `null` — they always return a `RelNode`. This is a low-impact defensive check that adds verbosity without addressing a realistic failure scenario.	Low

Suggestions up to commit cc90d7f

Category	Suggestion	Impact
Possible issue	Return new rule instances to avoid shared state Both `DatetimeUdtNormalizeRule` and `DatetimeOutputCastRule` extend `RelHomogeneousShuttle`, which is stateful (it tracks visited nodes). Returning shared `INSTANCE` singletons means concurrent or sequential queries will share and corrupt that state. Return new instances on each call instead. api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeExtension.java [23-26] @Override public List<RelShuttle> postAnalysisRules() { - return List.of(DatetimeUdtNormalizeRule.INSTANCE, DatetimeOutputCastRule.INSTANCE); + return List.of(new DatetimeUdtNormalizeRule(), new DatetimeOutputCastRule()); } Suggestion importance[1-10]: 7 __ Why: `RelHomogeneousShuttle` maintains a visited-node set as internal state, making shared singletons unsafe for concurrent or even sequential use. Returning new instances per call is a correct and important fix, and the `improved_code` accurately reflects the change needed.	Medium
	Prevent recursive application of output cast rule The rule visits every `RelNode` in the tree but only wraps the root — it should only apply to the top-level node. As written, it will also wrap intermediate nodes (e.g., inner `LogicalProject`s), causing redundant or incorrect double-casting of datetime fields. The rule should first recurse into children and then only apply the cast projection to the root, or it should be applied only once on the root outside the shuttle. api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeOutputCastRule.java [31-53] @Override public RelNode visit(RelNode other) { + // Do NOT recurse into children; this rule is intended for the root node only. List<RelDataTypeField> fields = other.getRowType().getFieldList(); if (fields.stream().noneMatch(f -> isDatetimeType(f.getType().getSqlTypeName()))) { return other; } - ... + + RexBuilder rexBuilder = other.getCluster().getRexBuilder(); + List<RexNode> projects = new ArrayList<>(fields.size()); + List<String> names = new ArrayList<>(fields.size()); + + for (RelDataTypeField field : fields) { + RexNode newField = rexBuilder.makeInputRef(other, field.getIndex()); + RelDataType fieldType = field.getType(); + if (isDatetimeType(fieldType.getSqlTypeName())) { + projects.add(castToVarchar(rexBuilder, newField, fieldType)); + } else { + projects.add(newField); + } + names.add(field.getName()); + } return LogicalProject.create(other, List.of(), projects, names); } Suggestion importance[1-10]: 5 __ Why: The concern about recursive application is valid in principle, but looking at the test cases (e.g., `testAllStandardDatetimeTypesCastToVarchar`), the expected plan shows the cast only on the outer `LogicalProject`, suggesting the current behavior may be intentional or the tests validate the actual behavior. The `improved_code` removes the call to `super.visit()` which would break child traversal needed by `DatetimeUdtNormalizeRule`. The suggestion has merit but the improved code doesn't accurately reflect a safe fix.	Low
	Avoid shared mutable shuttle singleton for thread safety `RelHomogeneousShuttle` (and `RelShuttle` in general) is not thread-safe because it maintains internal state (visited set). Sharing a single `INSTANCE` across concurrent queries will cause race conditions. Each call to `postAnalysisRules()` should return a new instance instead of a shared singleton. api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeUdtNormalizeRule.java [29] -static final DatetimeUdtNormalizeRule INSTANCE = new DatetimeUdtNormalizeRule(); +// Remove INSTANCE singleton; instantiate per use in DatetimeExtension.postAnalysisRules() +// In DatetimeExtension: +// return List.of(new DatetimeUdtNormalizeRule(), new DatetimeOutputCastRule()); Suggestion importance[1-10]: 3 __ Why: The thread-safety concern about `RelHomogeneousShuttle` is valid, but the `improved_code` is just a comment rather than actual code, making it an incomplete suggestion. The actual fix should be in `DatetimeExtension`, which is covered by suggestion 3.	Low

Suggestions up to commit 01fba06

Category	Suggestion	Impact
Possible issue	Recursively visit children before applying cast rule The rule does not recursively visit child nodes before applying the cast. If a datetime field appears in a nested node (not the root), it will be missed. Call `super.visit(other)` first to ensure children are visited before processing the current node. api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeOutputCastRule.java [31-36] @Override public RelNode visit(RelNode other) { - List<RelDataTypeField> fields = other.getRowType().getFieldList(); + RelNode visited = super.visit(other); + List<RelDataTypeField> fields = visited.getRowType().getFieldList(); if (fields.stream().noneMatch(f -> isDatetimeType(f.getType().getSqlTypeName()))) { - return other; + return visited; } + RexBuilder rexBuilder = visited.getCluster().getRexBuilder(); + List<RexNode> projects = new ArrayList<>(fields.size()); + List<String> names = new ArrayList<>(fields.size()); + for (RelDataTypeField field : fields) { + RexNode newField = rexBuilder.makeInputRef(visited, field.getIndex()); + RelDataType fieldType = field.getType(); + if (isDatetimeType(fieldType.getSqlTypeName())) { + projects.add(castToVarchar(rexBuilder, newField, fieldType)); + } else { + projects.add(newField); + } + names.add(field.getName()); + } + return LogicalProject.create(visited, List.of(), projects, names); +} Suggestion importance[1-10]: 6 __ Why: The `DatetimeOutputCastRule` doesn't call `super.visit(other)` before processing, meaning it only applies to the root node and misses datetime fields in nested nodes. The `DatetimeUdtNormalizeRule` correctly calls `super.visit(other)` first, so this inconsistency could be a real issue depending on the intended behavior. However, since this rule is described as wrapping "the root output", it may be intentional to only apply at the top level.	Low
General	Close ResultSet to prevent resource leaks The `ResultSet` is not closed after use, which can cause resource leaks. Wrap it in a try-with-resources block to ensure it is always closed. api/src/test/java/org/opensearch/sql/api/spec/datetime/DatetimeExtensionTest.java [167-174] -ResultSet resultSet = statement.executeQuery(); -verify(resultSet) - .expectSchema( - col("hire_date", VARCHAR), col("start_time", VARCHAR), col("created_at", VARCHAR)) - .expectData( - row("2024-01-16", "12:00:00", "2024-01-15 08:00:00"), - row("2024-06-20", "14:00:00", "2024-06-20 00:00:00")); +try (ResultSet resultSet = statement.executeQuery()) { + verify(resultSet) + .expectSchema( + col("hire_date", VARCHAR), col("start_time", VARCHAR), col("created_at", VARCHAR)) + .expectData( + row("2024-01-16", "12:00:00", "2024-01-15 08:00:00"), + row("2024-06-20", "14:00:00", "2024-06-20 00:00:00")); +} Suggestion importance[1-10]: 4 __ Why: The `ResultSet` is not closed after use, which can cause resource leaks. Wrapping it in a try-with-resources block is a valid improvement, though the impact is lower in test code since resources are typically cleaned up when the test JVM exits.	Low
General	Avoid shared mutable shuttle instances across queries `RelShuttle.accept` returns a `RelNode`, but `RelHomogeneousShuttle` implementations may return the same instance if no changes are made. However, the shuttles used here (`DatetimeUdtNormalizeRule`, `DatetimeOutputCastRule`) are stateful singletons shared across calls. If these shuttles accumulate any internal state between invocations, concurrent or sequential queries could interfere. Consider instantiating new shuttle instances per query rather than using shared `INSTANCE` singletons. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [66-70] +RelNode plan = strategy.plan(query); +for (var shuttle : context.getLangSpec().postAnalysisRules()) { + plan = plan.accept(shuttle); +} +return plan; - Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about singleton shuttle instances, but `RelHomogeneousShuttle` is typically stateless and the `improved_code` is identical to the `existing_code`, making this more of a verification suggestion than an actionable fix.	Low

Suggestions up to commit 70aabfe

Category	Suggestion	Impact
Possible issue	Missing recursive traversal of child nodes The `visit` method does not call `super.visit(other)` before processing the node, meaning child nodes are never recursively visited. This causes the rule to only apply to the root node and miss datetime fields in nested `RelNode`s. Call `super.visit(other)` first and operate on the returned node, consistent with how `DatetimeUdtNormalizeRule` handles this. api/src/main/java/org/opensearch/sql/api/spec/datetime/DatetimeOutputCastRule.java [28-32] public RelNode visit(RelNode other) { - List<RelDataTypeField> fields = other.getRowType().getFieldList(); + RelNode visited = super.visit(other); + List<RelDataTypeField> fields = visited.getRowType().getFieldList(); if (fields.stream().noneMatch(f -> isDatetimeType(f.getType().getSqlTypeName()))) { - return other; + return visited; } Suggestion importance[1-10]: 7 __ Why: The `DatetimeOutputCastRule.visit` method doesn't call `super.visit(other)` unlike `DatetimeUdtNormalizeRule`, meaning child nodes are never recursively visited. However, the rule is designed to wrap the root output with CAST for wire-format compatibility, so it may be intentionally applied only at the top level. The inconsistency with `DatetimeUdtNormalizeRule` is worth noting, but the impact depends on the intended behavior.	Medium
General	ResultSet resource not closed after use The `ResultSet` is never closed after use, which can leak JDBC resources. Wrap `resultSet` in a try-with-resources block or close it in a `finally` block to ensure proper cleanup. api/src/test/java/org/opensearch/sql/api/spec/datetime/DatetimeExtensionTest.java [167-174] -ResultSet resultSet = statement.executeQuery(); -verify(resultSet) - .expectSchema( - col("hire_date", VARCHAR), col("start_time", VARCHAR), col("created_at", VARCHAR)) - .expectData( - row("2024-01-16", "12:00:00", "2024-01-15 08:00:00"), - row("2024-06-20", "14:00:00", "2024-06-20 00:00:00")); +try (ResultSet resultSet = statement.executeQuery()) { + verify(resultSet) + .expectSchema( + col("hire_date", VARCHAR), col("start_time", VARCHAR), col("created_at", VARCHAR)) + .expectData( + row("2024-01-16", "12:00:00", "2024-01-15 08:00:00"), + row("2024-06-20", "14:00:00", "2024-06-20 00:00:00")); +} Suggestion importance[1-10]: 5 __ Why: The `ResultSet` is not closed after use, which can leak JDBC resources. The suggested fix of wrapping in try-with-resources is correct and the `improved_code` accurately reflects the change, though this is a test file so the impact is limited.	Low
General	Shared shuttle singletons risk concurrency issues `RelShuttle.accept` returns a `RelNode`, but `RelHomogeneousShuttle` instances are stateful — reusing the same `INSTANCE` singleton across multiple concurrent queries could cause race conditions if the shuttle holds any mutable state. Consider instantiating a new shuttle per invocation rather than using shared singletons (`DatetimeUdtNormalizeRule.INSTANCE`, `DatetimeOutputCastRule.INSTANCE`), or ensure the shuttles are truly stateless. api/src/main/java/org/opensearch/sql/api/UnifiedQueryPlanner.java [66-70] +RelNode plan = strategy.plan(query); +for (var shuttle : context.getLangSpec().postAnalysisRules()) { + plan = plan.accept(shuttle); +} +return plan; - Suggestion importance[1-10]: 4 __ Why: The concern about singleton `INSTANCE` shuttles being stateful is valid in principle, but the `improved_code` is identical to `existing_code`, making this more of a verification suggestion. The actual fix would need to be in `DatetimeExtension.postAnalysisRules()` to return new instances, not in `UnifiedQueryPlanner`.	Low

github-actions · 2026-05-06T18:33:48Z

Persistent review updated to latest commit 01fba06

dai-chen · 2026-05-06T18:42:22Z

All test related CI is failling on Gradle version issue:

* What went wrong:
A problem occurred evaluating root project 'opensearch-sql'.
> Failed to apply plugin class 'org.opensearch.gradle.info.GlobalBuildInfoPlugin'.
   > Gradle 9.4.1+ is required

github-actions · 2026-05-07T01:55:17Z

Persistent review updated to latest commit cc90d7f

dai-chen · 2026-05-07T02:07:43Z

All test related CI is failling on Gradle version issue:

* What went wrong:
A problem occurred evaluating root project 'opensearch-sql'.
> Failed to apply plugin class 'org.opensearch.gradle.info.GlobalBuildInfoPlugin'.
   > Gradle 9.4.1+ is required

Will rebase once #5414 merged.

Add postAnalysisRules (List<RelShuttle>) to LanguageSpec.LanguageExtension and register DatetimeExtension in UnifiedPplSpec with two rules: 1. DatetimeUdtNormalizeRule rewrites datetime UDT return types (EXPR_DATE/TIME/TIMESTAMP) on RexCall nodes to standard Calcite DATE/TIME(9)/TIMESTAMP(9) types via call.clone(). Precision is derived from the type system (OpenSearchTypeSystem.getMaxPrecision). 2. DatetimeOutputCastRule adds a final LogicalProject that casts standard datetime output columns to VARCHAR, aligning with PPL's wire-format contract (ISO string representation). Both rules run as postAnalysisRules after the planning strategy produces the RelNode, applied uniformly to both SQL and PPL paths. Also bumps OpenSearchTypeSystem max datetime precision from 3 to 9 (nanosecond) for TIME and TIMESTAMP types. No changes to UDF definitions or implementors in core/ — the mismatch between rewritten signatures and UDF implementations is a known limitation addressed separately. Signed-off-by: Chen Dai <[email protected]>

github-actions · 2026-05-07T02:55:27Z

Persistent review updated to latest commit af2b288

dai-chen self-assigned this May 6, 2026

dai-chen added PPL Piped processing language bugFix labels May 6, 2026

dai-chen force-pushed the feature/ppl-udt-cleanup branch from 70aabfe to 01fba06 Compare May 6, 2026 18:31

dai-chen marked this pull request as ready for review May 6, 2026 18:32

dai-chen requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, ahkcs, anirudha, joshuali925, mengweieric, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners May 6, 2026 18:32

penghuo previously approved these changes May 6, 2026

View reviewed changes

mengweieric previously approved these changes May 6, 2026

View reviewed changes

dai-chen dismissed stale reviews from mengweieric and penghuo via cc90d7f May 7, 2026 01:53

dai-chen force-pushed the feature/ppl-udt-cleanup branch from 01fba06 to cc90d7f Compare May 7, 2026 01:53

dai-chen force-pushed the feature/ppl-udt-cleanup branch from cc90d7f to af2b288 Compare May 7, 2026 02:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize datetime types for unified query API#5408

Normalize datetime types for unified query API#5408
dai-chen wants to merge 1 commit intoopensearch-project:mainfrom
dai-chen:feature/ppl-udt-cleanup

dai-chen commented May 6, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 6, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 6, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 6, 2026

Uh oh!

dai-chen commented May 6, 2026

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

dai-chen commented May 7, 2026

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

dai-chen commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Examples

Notes

Related Issues

Check List

Uh oh!

github-actions Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit af2b288)

Uh oh!

github-actions Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented May 6, 2026

Uh oh!

dai-chen commented May 6, 2026

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

dai-chen commented May 7, 2026

Uh oh!

github-actions Bot commented May 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dai-chen commented May 6, 2026 •

edited

Loading

github-actions Bot commented May 6, 2026 •

edited

Loading

(Review updated until commit `af2b288`)

github-actions Bot commented May 6, 2026 •

edited

Loading